-
Notifications
You must be signed in to change notification settings - Fork 22
Feat/service task #745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/service task #745
Conversation
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Process/ServiceTasks/EFormidling/EFormidlingServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Api.Tests/Process/ServiceTasks/EFormidling/EFormidlingServiceTaskTests.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Process/ServiceTasks/EFormidling/EFormidlingServiceTaskTests.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Process/ServiceTasks/Pdf/PdfServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Api.Tests/Process/ServiceTasks/Pdf/PdfServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API and stuff LGTM. We discussed
- Validation error for trying to invoke actions when current task is a service task
- Clear/simple migration story (both old and new apps work) - can we just not run the legacy tasks if a PDF service task refers to the current task for example
- Service tasks failing should probably not have 500 status code for the user/client?
- Talk to frontenders and Studio team on how the task should be configured
7584f9a to
6aacedf
Compare
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Outdated
Show resolved
Hide resolved
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
Outdated
Show resolved
Hide resolved
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
Outdated
Show resolved
Hide resolved
|
|
1 similar comment
|
|
1d3fd8d to
9ec36c7
Compare
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Process/ServiceTasks/EFormidling/EFormidlingServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Api.Tests/Process/ServiceTasks/Pdf/PdfServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
...Altinn.App.Core.Tests/Internal/Process/EventHandlers/ProcessTask/EndTaskEventHandlerTests.cs
Dismissed
Show dismissed
Hide dismissed
...Altinn.App.Core.Tests/Internal/Process/EventHandlers/ProcessTask/EndTaskEventHandlerTests.cs
Dismissed
Show dismissed
Hide dismissed
6ef10b3 to
9b6b142
Compare
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
Fixed
Show fixed
Hide fixed
bb80fbe to
70a69eb
Compare
|
/publish |
PR release:
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (1)
19-21: Consider consistent null handling.The property is marked nullable (
List<string>?) withIsNullable = true, but defaults to a non-null empty list[]. This creates ambiguity about whethernullis a valid state. Consider either:
- Removing the default initializer to allow
nullas the explicit "not configured" state, or- Making the property non-nullable if an empty list is always valid
The current code works, but explicit semantics would improve clarity.
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
259-293: Consider logging when text resource key is not found.Lines 265-275: When
fileNameTextResourceElementIdis provided but the corresponding text resource is not found, the code falls back to using the key itself as the filename (line 274). This might produce unexpected filenames like"pdf.filename.key.pdf".Consider adding a warning log before line 274 to help identify misconfigurations:
if (textResourceElement is not null) return GetValidFileName(textResourceElement.Value); // Log warning about missing text resource _logger.LogWarning("Text resource key '{Key}' not found, using key as filename.", fileNameTextResourceElementId); return GetValidFileName(fileNameTextResourceElementId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs(1 hunks)src/Altinn.App.Core/Internal/Pdf/PdfService.cs(9 hunks)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Internal/Pdf/IPdfService.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Internal/Pdf/IPdfService.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Pdf/IPdfService.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
🧠 Learnings (4)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
PR: Altinn/app-lib-dotnet#1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧬 Code graph analysis (5)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (1)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
List(400-415)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs (6)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (3)
PdfServiceTask(13-67)PdfServiceTask(22-27)Task(33-52)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-76)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (14)
AltinnTaskExtension(209-221)List(41-45)List(48-52)List(62-66)List(69-73)List(83-87)List(90-94)List(97-101)List(104-108)List(118-122)List(125-129)List(165-182)List(185-194)List(197-206)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (1)
AltinnPdfConfiguration(8-29)src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (4)
Task(17-17)Task(28-34)Task(42-42)Task(55-55)src/Altinn.App.Core/Internal/Pdf/PdfService.cs (8)
Task(71-76)Task(79-96)Task(99-112)Task(115-118)Task(120-155)Task(157-193)Task(243-257)List(400-415)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (4)
src/Altinn.App.Core/Internal/Process/Elements/Process.cs (1)
Process(8-62)src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
List(400-415)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (13)
List(41-45)List(48-52)List(62-66)List(69-73)List(83-87)List(90-94)List(97-101)List(104-108)List(118-122)List(125-129)List(165-182)List(185-194)List(197-206)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
ValidAltinnPdfConfiguration(54-66)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (4)
Task(17-17)Task(28-34)Task(42-42)Task(55-55)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (4)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (3)
Task(17-17)Task(28-34)Task(42-42)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (2)
ValidAltinnPdfConfiguration(23-28)AltinnPdfConfiguration(8-29)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-76)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (1)
AltinnTaskExtension(209-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (18)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (2)
15-15: Documentation typo fixes look good.The corrections from "witch" to "which" improve the documentation quality.
Also applies to: 40-40
19-34: No action required:PdfServiceexplicitly overrides the new overload and forwards all parameters, so the default implementation is never used and there’s no silent parameter ignore.src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (3)
8-14: LGTM!The class is correctly marked as
sealed, follows the coding guidelines, and theFilenameproperty is properly configured with XML serialization attributes to support text resource keys.
23-28: LGTM!The
Validate()method correctly normalizesFilenameby trimming and converting whitespace-only values tonull, which prevents empty strings from being passed downstream. TheAutoPdfTaskIdslist is passed through as-is, which is appropriate for a collection property.
31-31: LGTM!The internal
readonly record structis an appropriate and efficient choice for encapsulating validated configuration data.src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (3)
8-17: LGTM!The class is correctly marked as
internal sealedper coding guidelines, and the constructor dependencies are appropriate for the PDF generation task.
33-52: Confirm taskId is not user-controlled input.Static analysis flagged Lines 38 and 49 for logging user-provided values. The
taskIdcomes frominstance.Process.CurrentTask.ElementId, which should be a BPMN process model identifier controlled by the app developer rather than end-user input.Please confirm this is the case to dismiss the security concern.
54-66: LGTM!The helper method correctly handles both configured and unconfigured scenarios, providing a sensible default when no PDF configuration is specified in the BPMN model. The null-safe navigation and validation flow are appropriate.
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs (3)
12-34: LGTM!The test setup follows good practices with inline mock initialization and appropriate default configuration for the ProcessReader mock. The structure supports test reusability.
36-65: LGTM!The test correctly verifies that
ExecutecallsGenerateAndStorePdfwith the expected parameters. The use ofIt.IsAny<>()forAutoPdfTaskIdsandCancellationTokenis appropriate given this test focuses on the core invocation.
67-103: LGTM!The test correctly verifies that
AutoPdfTaskIdsare propagated through toGenerateAndStorePdf. Creating a separatePdfServiceTaskinstance for this test is appropriate to avoid interference with the default setup.src/Altinn.App.Core/Internal/Pdf/PdfService.cs (7)
70-76: LGTM!The original method now correctly delegates to
GenerateAndStorePdfInternalwithnullfor the new parameters, maintaining backward compatibility.
78-96: LGTM!The new overload correctly extends the public API to support filename and auto-PDF task configuration, with appropriate default parameter values.
120-155: LGTM!The internal method correctly consolidates the PDF generation and storage logic, properly disposes the stream with
await using, and passes through the extended configuration parameters appropriately.
99-111: LGTM!The changes correctly integrate
autoGeneratePdfForTaskIdsinto the PDF content generation flow, converting task IDs to query parameters and appending them to the URI.Also applies to: 157-193
195-223: LGTM!The
BuildUrimethod correctly handles additional query parameters, and the string concatenation approach was acknowledged as acceptable by the maintainer for this use case.
339-347: LGTM!The helper correctly ensures the
400-415: LGTM!The helper correctly converts task IDs into query parameters with repeated
taskkeys, which is the standard approach for array-like query parameters in URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
1-7: Missing using directive prevents compilation.The
IProcessReadertype (line 17) requires a namespace import. This issue was flagged in a previous review.Apply this diff:
using Altinn.App.Core.Helpers; using Altinn.App.Core.Internal.Pdf; +using Altinn.App.Core.Internal.Process; using Altinn.App.Core.Internal.Process.Elements.AltinnExtensionProperties; using Altinn.Platform.Storage.Interface.Models; using Microsoft.Extensions.Logging;
🧹 Nitpick comments (1)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
427-432: Clarify error handling for service task implementation bugs.If a service task fails to handle abandon issues, the thrown exception will be caught by the generic catch block (line 440), which logs "Service task failed with an exception!" This masks the specific implementation error. Consider either:
- Rethrowing this exception type so it's not caught by the generic handler, or
- Logging a more specific message before throwing
This would help distinguish between service task execution failures and implementation bugs.
Apply this diff to log a specific message before throwing:
if (cachedDataMutator.HasAbandonIssues) { + _logger.LogError( + "Service task {ServiceTaskType} implementation error: Abandon issues found in data elements but not handled by the service task.", + serviceTask.Type + ); throw new Exception( "Abandon issues found in data elements. Abandon issues should be handled by the service task." ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs(11 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
🧠 Learnings (3)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-25T11:15:19.192Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: src/Altinn.App.Core/Internal/Process/ProcessEngine.cs:276-276
Timestamp: 2025-09-25T11:15:19.192Z
Learning: In ProcessEngine.Next(), HandleMoveToNext intentionally uses request.Action (original, potentially null) rather than checkedAction (resolved with defaults). This distinction is important for navigation logic, even though it may appear inconsistent with how authorization and service task execution use the resolved action.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
🧬 Code graph analysis (3)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (7)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs (2)
Task(44-77)Task(79-95)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
Task(34-56)src/Altinn.App.Api/Controllers/ProcessController.cs (3)
Task(479-519)Task(623-653)Task(655-658)src/Altinn.App.Core/Helpers/LogSanitizer.cs (2)
LogSanitizer(9-45)Sanitize(22-38)src/Altinn.App.Core/Internal/Process/ProcessReaderExtensions.cs (1)
IsActionAllowedForTask(13-23)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
InstanceDataUnitOfWork(24-701)InstanceDataUnitOfWork(65-98)DataElementChanges(351-428)src/Altinn.App.Core/Models/Validation/ValidationIssueWithSource.cs (2)
ValidationIssueWithSource(10-118)ValidationIssueWithSource(15-32)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs (6)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (12)
IServiceTask(313-324)Task(74-141)Task(144-292)Task(295-310)Task(326-378)Task(380-453)Task(458-487)Task(492-514)Task(516-588)Task(590-643)Task(645-658)Task(663-676)src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs (3)
Task(71-74)Task(77-80)Task(82-138)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/IServiceTask.cs (1)
Task(14-14)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnEFormidlingConfiguration.cs (2)
ValidAltinnEFormidlingConfiguration(66-96)AltinnEFormidlingConfiguration(10-199)src/Altinn.App.Core/Helpers/LogSanitizer.cs (2)
LogSanitizer(9-45)Sanitize(22-38)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (1)
AltinnTaskExtension(209-221)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (5)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (3)
Task(17-17)Task(28-34)Task(42-42)src/Altinn.App.Core/Helpers/LogSanitizer.cs (2)
LogSanitizer(9-45)Sanitize(22-38)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (2)
ValidAltinnPdfConfiguration(23-28)AltinnPdfConfiguration(8-29)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-76)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (1)
AltinnTaskExtension(209-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (5)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (2)
703-760: LGTM: Well-structured validation helper.The
TryGetCurrentTaskIdAndAltinnTaskTypemethod follows C# conventions properly, uses nullability attributes correctly, and provides clear error messages for each validation case. The extraction of this logic from the mainNextmethod improves readability and maintainability.
144-292: LGTM: Well-structured refactor of the Next method.The refactored
Nextmethod follows a clear flow: authorization → service task or user action execution → conditional validation → process advancement. Key improvements:
- Authorization and validation are properly integrated
- Service tasks and user actions are routed correctly
- Error handling provides specific error types and messages
- Telemetry tracks each phase
- Validation is appropriately skipped for reject actions and service tasks
The logic is well-organized and maintains backward compatibility while adding new capabilities.
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (3)
39-39: Static analysis warnings are false positives.The static analysis tool flagged logging
taskIdas a security risk (log injection). However, the code correctly appliesLogSanitizer.Sanitize()before logging, which removes control characters and limits length, mitigating injection risks.Also applies to: 50-53
36-36: Remove null check suggestion for CurrentTask.ElementIdService task contexts always supply a non-null
CurrentTaskand the codebase consistently accessesElementIddirectly, so a null check here is unnecessary.Likely an incorrect or invalid review comment.
58-70: Exception handling for missing task extensions is consistent. All service tasks propagate theProcessExceptionfromGetAltinnTaskExtensionand default null configurations.
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Show resolved
Hide resolved
|
/publish |
PR release:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Altinn.App.Api/Controllers/ProcessController.cs (1)
47-68: Avoid service locator; inject InstanceDataUnitOfWorkInitializer directlyThe constructor still uses
IServiceProviderto resolveInstanceDataUnitOfWorkInitializeron line 67. This anti-pattern hides dependencies and complicates testing.As per the previous review, make
InstanceDataUnitOfWorkInitializeraccessible for direct injection by either:
- Making the class
publicinsrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWorkInitializer.cs, OR- Adding
[assembly: InternalsVisibleTo("Altinn.App.Api")]in CoreThen update the constructor:
public ProcessController( ILogger<ProcessController> logger, IInstanceClient instanceClient, IProcessClient processClient, IValidationService validationService, IAuthorizationService authorization, IProcessReader processReader, IProcessEngine processEngine, - IServiceProvider serviceProvider, + InstanceDataUnitOfWorkInitializer instanceDataUnitOfWorkInitializer, IProcessEngineAuthorizer processEngineAuthorizer ) { _logger = logger; _instanceClient = instanceClient; _processClient = processClient; _authorization = authorization; _processReader = processReader; _processEngine = processEngine; _processEngineAuthorizer = processEngineAuthorizer; _validationService = validationService; - _instanceDataUnitOfWorkInitializer = serviceProvider.GetRequiredService<InstanceDataUnitOfWorkInitializer>(); + _instanceDataUnitOfWorkInitializer = instanceDataUnitOfWorkInitializer; }
🧹 Nitpick comments (1)
src/Altinn.App.Api/Controllers/ProcessController.cs (1)
508-556: Consider exhaustive pattern matching for ProcessErrorTypeThe centralized error handling is well-designed. For additional safety, consider using an exhaustive switch expression or adding a guard clause to ensure all
ProcessErrorTypeenum values are explicitly handled:private ActionResult<AppProcessState> GetResultForError(ProcessChangeResult result) { - switch (result.ErrorType) + return result.ErrorType switch { - case ProcessErrorType.Conflict: - return Conflict( + ProcessErrorType.Conflict => Conflict( new ProblemDetails() { Detail = result.ErrorMessage, Status = StatusCodes.Status409Conflict, Title = result.ErrorTitle, Extensions = new Dictionary<string, object?> { { "validationIssues", result.ValidationIssues }, }, } ), - case ProcessErrorType.Internal: - return StatusCode( + ProcessErrorType.Internal => StatusCode( 500, new ProblemDetails() { Detail = result.ErrorMessage, Status = StatusCodes.Status500InternalServerError, Title = result.ErrorTitle ?? "Internal server error", } ), - case ProcessErrorType.Unauthorized: - return StatusCode( + ProcessErrorType.Unauthorized => StatusCode( 403, new ProblemDetails() { Detail = result.ErrorMessage, Status = StatusCodes.Status403Forbidden, Title = result.ErrorTitle ?? "Unauthorized", } ), - default: - return StatusCode( + _ => StatusCode( 500, new ProblemDetails() { Detail = $"Unknown ProcessErrorType {result.ErrorType}", Status = StatusCodes.Status500InternalServerError, Title = result.ErrorTitle ?? "Internal server error", } - ); - } + ), + }; }This makes the compiler warn if new enum values are added without updating the switch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Api/Controllers/ProcessController.cs(9 hunks)src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs(2 hunks)test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_FailingValidator_ReturnsValidationErrors.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_PdfFails_DataIsUnlocked.verified.txt(0 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(18 hunks)
💤 Files with no reviewable changes (1)
- test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_PdfFails_DataIsUnlocked.verified.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
- test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_FailingValidator_ReturnsValidationErrors.verified.txt
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Api/Controllers/ProcessController.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Api/Controllers/ProcessController.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Api/Controllers/ProcessController.cs
🧠 Learnings (3)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-25T11:15:19.192Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: src/Altinn.App.Core/Internal/Process/ProcessEngine.cs:276-276
Timestamp: 2025-09-25T11:15:19.192Z
Learning: In ProcessEngine.Next(), HandleMoveToNext intentionally uses request.Action (original, potentially null) rather than checkedAction (resolved with defaults). This distinction is important for navigation logic, even though it may appear inconsistent with how authorization and service task execution use the resolved action.
Applied to files:
src/Altinn.App.Api/Controllers/ProcessController.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧬 Code graph analysis (1)
src/Altinn.App.Api/Controllers/ProcessController.cs (5)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWorkInitializer.cs (2)
InstanceDataUnitOfWorkInitializer(15-71)InstanceDataUnitOfWorkInitializer(29-48)src/Altinn.App.Core/Models/Process/ProcessChangeResult.cs (1)
ProcessChangeResult(9-42)src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (13)
ConvertTaskTypeToAction(755-772)Task(80-147)Task(150-213)Task(218-349)Task(366-381)Task(397-449)Task(451-524)Task(529-558)Task(563-585)Task(587-659)Task(661-714)Task(716-729)Task(734-747)src/Altinn.App.Core/Internal/Process/Elements/ProcessTask.cs (2)
ElementType(21-24)ProcessTask(9-25)src/Altinn.App.Core/Internal/Process/Elements/AppProcessTaskTypeInfo.cs (1)
AppProcessTaskTypeInfo(10-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
🔇 Additional comments (5)
src/Altinn.App.Api/Controllers/ProcessController.cs (5)
256-303: Simplified flow correctly delegates iteration to ProcessEngineThe
NextElementmethod now constructs a singleProcessNextRequestand delegates to_processEngine.Next(), which handles iterative service-task execution internally. This resolves the stale-state loop concern from previous reviews where the controller was checkingIsServiceTaskon the pre-Next instance.The structured logging improvement on line 296 is also a good practice.
612-642: Validation helper correctly supports CompleteProcess flowThe
GetValidationProblemDetailsmethod provides necessary pre-validation for theCompleteProcessloop (lines 366-409). While similar logic exists inProcessEngine.ProcessNext, this duplication is intentional:CompleteProcessmust validate before each iteration to determine whether to continue the loop, whereasProcessNextvalidates internally as part of the process change.The method correctly uses
InstanceDataUnitOfWorkInitializerandIValidationService, and formats errors consistently withGetResultForError.
391-398: LGTM: Clean ProcessNextRequest constructionThe use of target-typed
new()and proper action conversion viaConvertTaskTypeToActionis clean and idiomatic.
486-501: ElementType wiring supports new BPMN surfaceThe additions of
ElementTypetoCurrentTask(line 486) andProcessTasks(line 497) correctly populate the new API surface properties. This aligns with the PR's service-task feature and enables element-type-based behavior in the frontend.The use of
flowElement.ElementType()andprocessElement.ElementType()properly delegates to the process element model's type identification.
649-666: Action mapping correctly extends for service tasksAdding
"pdf"and"eFormidling"to the action conversion is consistent with the engine-side mapping (seeProcessEngine.ConvertTaskTypeToAction) and appropriately maps these service-task types to the"write"action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (4)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (4)
3582-3587: IProcessEngine additions are breaking for implementers — use default interface bodies.This was already flagged earlier; ensure default bodies exist to preserve compat (especially
CheckIfServiceTaskand oldNextsignature forwarding).#!/bin/bash ast-grep --pattern $'interface IProcessEngine { $$$ }' --lang cs -n -C3
3628-3633: ProcessEngine ctor expanded — add an [Obsolete] overload that forwards.Previously raised; confirm a compat ctor exists to avoid binary/source breaks.
#!/bin/bash rg -nP --type=cs 'class\s+ProcessEngine\b' -C5
3381-3383: elementType casing consistency (lowerCamelCase).Already noted earlier; ensure overrides return lowerCamelCase to match JSON/client expectations.
Also applies to: 3405-3406
4705-4705: Plumb new fields to API responses and telemetry.Ensure ErrorTitle and ValidationIssues are wired through controllers and telemetry tags; previously raised.
Also applies to: 4721-4722
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(18 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (2)
4737-4744: Verify whether ProcessNextRequest sealing is a new change in this PR.ProcessNextRequest is currently defined as
public sealed record. I cannot determine from the codebase alone whether sealing is new in this PR or was pre-existing. If the sealing is a new addition in this PR, the concern about breaking inheritance is valid—sealed records prevent consumers from deriving custom implementations. If it was already sealed, no action is needed.Check the PR diff to confirm: (1) was ProcessNextRequest previously unsealed, and (2) is this sealing intentional per versioning policy (breaking change acceptable for next major version)?
191-199: ****The interfaces
IEFormidlingReceiversandIEFormidlingServicealready have default implementations for the new overload methods. The secondGetEFormidlingReceiversoverload forwards to the first method, and the secondSendEFormidlingShipmentoverload forwards to the first method with an explicit backward-compatibility comment. BothDefaultEFormidlingReceiversandDefaultEFormidlingServicesuccessfully implement all overloads without compilation issues. No breaking change exists, and no modifications are needed.Likely an incorrect or invalid review comment.
.../Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Show resolved
Hide resolved
# Conflicts: # src/Altinn.App.Core/Internal/Pdf/PdfService.cs
|
/publish |
PR release:
|
|
|
/publish |
PR release:
|



Implementing basic support for service task in bpmn.
Moving PDF and eFormidling into service tasks, but also keep legacy implementations and old way of configuring for backwards compatibility for now.
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Tests